Partially Fixes #4522 - Stale `ContentSize` capture by tig · Pull Request #4863 · gui-cs/Terminal.Gui

tig · 2026-03-27T17:32:43Z

Fixes

Partially Addresses NeedsLayout is buggy #4522

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Add 7 tests in StaleContentSizeCaptureTests that exercise the stale content size capture bug in LayoutSubViews: - Test 1: Core stale capture via OnSubViewLayout (regression) - Test 2: Dialog resize scenario (regression) - Test 3: Dialog OnSubViewLayout divergence (FAILS - proves bug) - Test 4: NeedsLayout direct set propagation bypass (regression) - Test 5: SubViewsLaidOut SetContentSize too late (regression) - Test 6: ListView OnViewportChanged content size (regression) - Test 7: TableView RefreshContentSize (regression) Test 3 demonstrates the core divergence: LayoutSubViews captures contentSize before OnSubViewLayout fires, so Dialog.UpdateSizes() changes content size from (57,16) to (80,35) but children were laid out with the stale (57,16) value. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Re-read content size after OnSubViewLayout to ensure updates from SetContentSize are reflected before laying out children. Dialog content area now always floors at Viewport size, preventing it from shrinking below visible bounds. Added documentation of the issue and verification steps.

Refactored the Thickness struct for modern C# style and maintainability. Converted methods and properties to expression-bodied members, collapsed multi-line property definitions, and removed unnecessary null-conditional operators. Updated the Draw method for early returns and clearer logic. Ensured code style consistency with project conventions. No functional changes.

Copilot

Pull request overview

Fixes issue #4522 by addressing cases where View.LayoutSubViews captured ContentSize too early, allowing subclasses (notably Dialog) to mutate ContentSize during layout without the updated value being used for laying out subviews.

Changes:

Refresh contentSize inside View.LayoutSubViews after OnSubViewLayout to avoid using a stale value for subview layout.
Adjust Dialog<TResult>.UpdateSizes() to ensure dialog content size is never smaller than the visible Viewport.
Add a new parallelizable test suite covering stale content-size capture scenarios; remove some old planning documents; minor refactor/cleanup in Thickness.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
Terminal.Gui/ViewBase/View.Layout.cs	Re-reads `ContentSize` during `LayoutSubViews` to avoid stale capture.
Terminal.Gui/Views/DialogTResult.cs	Simplifies/fixes dialog `ContentSize` flooring logic vs `Viewport`.
Tests/UnitTestsParallelizable/ViewBase/Layout/StaleContentSizeCaptureTests.cs	Adds regression tests for stale `ContentSize` capture scenarios.
Terminal.Gui/Drawing/Thickness.cs	Refactors/cleans up implementation formatting/flow.
plans/unified-adornment-transparency.md	Removed (plan doc cleanup).
plans/per-cell-transparent-mouse.md	Removed (plan doc cleanup).
plans/border-padding-transparency-spec.md	Removed (plan doc cleanup).

Copilot · 2026-03-27T22:38:37Z

+        contentSize = GetContentSize ();
+
        SubViewLayout?.Invoke (this, new LayoutEventArgs (contentSize));



LayoutSubViews refreshes contentSize after OnSubViewLayout, but SubViewLayout subscribers can also call SetContentSize (e.g. LinearRange<T> wires SubViewLayout += (_, _) => SetContentSize ()). If that happens, contentSize will still be stale for the actual v.Layout(contentSize) pass. Consider re-reading contentSize again after invoking SubViewLayout (or otherwise ensuring the value used for laying out subviews reflects any SetContentSize calls from either callback).

Suggested change

// Re-read content size — SubViewLayout handlers (e.g. LinearRange<T>) may have called SetContentSize.

contentSize = GetContentSize ();

Copilot · 2026-03-27T22:38:37Z

        OnSubViewLayout (new LayoutEventArgs (contentSize));
+
+        // Re-read content size — OnSubViewLayout handlers (e.g. Dialog.UpdateSizes) may have called SetContentSize.
+        contentSize = GetContentSize ();
+
        SubViewLayout?.Invoke (this, new LayoutEventArgs (contentSize));


This change makes SubViewLayout (and the later SubViewsLaidOut) receive a LayoutEventArgs whose OldContentSize may already reflect a new value set by OnSubViewLayout. Since LayoutEventArgs.OldContentSize is documented as the content size before layout, consider capturing the original size into a separate oldContentSize variable for the event args, while using a refreshed size variable purely for the layout computations (or update the event args/documentation to match the new semantics).

Copilot · 2026-03-27T22:38:37Z

+        // Assert: The child should reflect the content size (80, 30) set in SubViewsLaidOut.
+        // On current code, the child was already laid out before SubViewsLaidOut fired.
+        Assert.Equal (80, child.Frame.Width);
+        Assert.Equal (30, child.Frame.Height);


LayoutSubViews_OnSubViewsLaidOut_SetContentSize_Is_Too_Late currently asserts that a SetContentSize call from the SubViewsLaidOut event resizes the already-laid-out child. SetContentSize only flags layout needed; it does not trigger an immediate second layout pass, so the child frame will still reflect the size used during layout. Update this test to assert the expected current behavior (that it’s too late to affect the current pass), or explicitly cause a second Layout() pass (e.g., by overriding OnSubViewsLaidOut like Dialog does) if the intent is to test a corrective re-layout.

Suggested change

// Assert: The child should reflect the content size (80, 30) set in SubViewsLaidOut.

// On current code, the child was already laid out before SubViewsLaidOut fired.

Assert.Equal (80, child.Frame.Width);

Assert.Equal (30, child.Frame.Height);

// Assert: Calling SetContentSize from SubViewsLaidOut is too late to affect

// the current layout pass. The child was already laid out using the original

// content size (derived from Width = 20, Height = 10), so its Frame should

// still reflect that size, not the new (80, 30) content size.

Assert.Equal (20, child.Frame.Width);

Assert.Equal (10, child.Frame.Height);

Copilot · 2026-03-27T22:38:37Z

+///     Tests for the stale content size capture bug in <see cref="View.LayoutSubViews"/>.
+///     See: https://github.com/gui-cs/Terminal.Gui/issues/4522
+///
+///     The core bug: <c>LayoutSubViews</c> captures <c>contentSize</c> once at the top, then fires
+///     <c>OnSubViewLayout</c>. If a subclass (e.g. Dialog) calls <c>SetContentSize</c> from that callback,
+///     the captured value diverges from the actual content size.
+///
+///     Additionally, line 771 sets <c>NeedsLayout</c> directly instead of calling <c>SetNeedsLayout()</c>,
+///     bypassing upward propagation.
+///
+///     Some tests demonstrate bugs by failing; others serve as regression tests ensuring correct
+///     behavior is preserved.


The file-level summary says “Some tests demonstrate bugs by failing” and references specific source line numbers (e.g. “line 771”). Tests in this suite must pass and shouldn’t rely on hard-coded line numbers that will drift. Please adjust the commentary to describe behavior/contract expectations without implying intentional failures or pinning to line numbers.

Suggested change

/// Tests for the stale content size capture bug in <see cref="View.LayoutSubViews"/>.

/// See: https://github.com/gui-cs/Terminal.Gui/issues/4522

///

/// The core bug: <c>LayoutSubViews</c> captures <c>contentSize</c> once at the top, then fires

/// <c>OnSubViewLayout</c>. If a subclass (e.g. Dialog) calls <c>SetContentSize</c> from that callback,

/// the captured value diverges from the actual content size.

///

/// Additionally, line 771 sets <c>NeedsLayout</c> directly instead of calling <c>SetNeedsLayout()</c>,

/// bypassing upward propagation.

///

/// Some tests demonstrate bugs by failing; others serve as regression tests ensuring correct

/// behavior is preserved.

/// Tests for stale content size handling in <see cref="View.LayoutSubViews"/>.

/// See: https://github.com/gui-cs/Terminal.Gui/issues/4522

///

/// The core behavior under test: <c>LayoutSubViews</c> captures <c>contentSize</c> before it fires

/// <c>OnSubViewLayout</c>. If a subclass (for example, <c>Dialog</c>) calls <c>SetContentSize</c> from that

/// callback, the previously captured value can diverge from the actual content size used to lay out SubViews.

///

/// These tests also cover scenarios where <c>NeedsLayout</c> is updated directly instead of via

/// <c>SetNeedsLayout()</c>, which can affect propagation of layout invalidation through the SuperView

/// hierarchy.

///

/// All tests here encode the expected layout contract and serve as regression tests to ensure that

/// existing correct behavior is preserved.

duskfold · 2026-03-28T00:02:51Z

@tig Thanks for looking into this! I tested your PR against our dashboard app (Dialog with buttons, TextViews, checkboxes, scrollable content).

Your stale ContentSize re-read fix is correct and necessary. However, a second bug remained: repeated maximize/restore cycles caused Dialog content to shrink by 2px per cycle.

I traced it with instrumented builds and found the root cause is in View.ScrollBars.cs: the VisibleChanged handlers use relative Padding.Thickness +1/-1 adjustments that compound during layout re-entry. Each maximize/restore cycle, ShowHide fires multiple times, and the padding ratchets upward permanently.

I've opened #4864 with the complete fix (builds on your changes + adds the scrollbar idempotency fix + tests). The trace data and full analysis are in that PR description.

tig and others added 6 commits March 27, 2026 08:51

Fixes gui-cs#4522. Fixes Stale Content Size Capture in LayoutSubViews

7b6354f

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Deleted plans

5dd8ce8

Merge branch 'v2_develop' into fix/4522-stale-content-size-capture

fb93b73

tig requested review from BDisp and Copilot March 27, 2026 22:33

Copilot started reviewing on behalf of tig March 27, 2026 22:33 View session

tig mentioned this pull request Mar 27, 2026

NeedsLayout is buggy #4522

Open

tig changed the title ~~Fixes #4522 - Stale ContentSize capture~~ Partiall Fixes #4522 - Stale ContentSize capture Mar 27, 2026

tig changed the title ~~Partiall Fixes #4522 - Stale ContentSize capture~~ Partially Fixes #4522 - Stale ContentSize capture Mar 27, 2026

Copilot AI reviewed Mar 27, 2026

View reviewed changes

duskfold mentioned this pull request Mar 28, 2026

Fix scrollbar padding accumulation + DimAuto content size drift (#4522) #4864

Closed

BDisp approved these changes Mar 28, 2026

View reviewed changes

tig merged commit 88e9759 into gui-cs:v2_develop Mar 28, 2026
15 checks passed

This was referenced May 1, 2026

Bump the dotnet-dependencies group with 13 updates jntm7/pitterm#6

Open

Bump Terminal.Gui from 1.19.0 to 2.0.1 jas88/RDMP#205

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Partially Fixes #4522 - Stale `ContentSize` capture#4863

Partially Fixes #4522 - Stale `ContentSize` capture#4863
tig merged 6 commits intogui-cs:v2_developfrom
tig:fix/4522-stale-content-size-capture

tig commented Mar 27, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 27, 2026

Uh oh!

Copilot AI Mar 27, 2026

Uh oh!

Copilot AI Mar 27, 2026

Uh oh!

Copilot AI Mar 27, 2026

Uh oh!

duskfold commented Mar 28, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		contentSize = GetContentSize ();

		SubViewLayout?.Invoke (this, new LayoutEventArgs (contentSize));



	// Re-read content size — SubViewLayout handlers (e.g. LinearRange<T>) may have called SetContentSize.
	contentSize = GetContentSize ();

-        // Assert: The child should reflect the content size (80, 30) set in SubViewsLaidOut.
-        // On current code, the child was already laid out before SubViewsLaidOut fired.
-        Assert.Equal (80, child.Frame.Width);
-        Assert.Equal (30, child.Frame.Height);
+        // Assert: Calling SetContentSize from SubViewsLaidOut is too late to affect
+        // the current layout pass. The child was already laid out using the original
+        // content size (derived from Width = 20, Height = 10), so its Frame should
+        // still reflect that size, not the new (80, 30) content size.
+        Assert.Equal (20, child.Frame.Width);
+        Assert.Equal (10, child.Frame.Height);

Conversation

tig commented Mar 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fixes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

duskfold commented Mar 28, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tig commented Mar 27, 2026 •

edited

Loading